Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

entproto: fix service.List pageToken for custom fields (UUID/string) #600

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kdevo
Copy link
Contributor

@kdevo kdevo commented Aug 3, 2024

Motivation

The current List service template generates faulty code if a custom GoType field is used (currently only UUID and string types are supported by this fix).

I haven't tested this on a large set of inputs and probably I'm lacking context why the pageToken has been implemented this way (also why it is Base64-encoded), but using a call to the field_to_ent template hopefully leads to a more sane behavior, as it is also used for Get() and Update() templates.

Example schema

Example 1: UUID type

		field.UUID("id", PULID{}).
			Default(func() PULID { return MustNewPULID(m.prefix) }).
			Annotations(
				entproto.Field(1)),
			),

Example 2: String type

		field.Bytes("id").
		 	GoType(PULID{}).
		 	DefaultFunc(func() PULID { return MustNewPULID(m.prefix) }).
		 	Annotations(
		 		entproto.Field(1, entproto.Type(descriptorpb.FieldDescriptorProto_TYPE_STRING)),
		 	).

Diff of rendered code

IDLTE only accepts custom type (in this case pulid.PULID). Therefore the green lines are working code:

	if req.GetPageToken() != "" {
		bytes, err := base64.StdEncoding.DecodeString(req.PageToken)
		if err != nil {
			return nil, status.Errorf(codes.InvalidArgument, "page token is invalid")
		}
-		pageToken, err := uuid.ParseBytes(bytes)
-		if err != nil {
-			return nil, status.Errorf(codes.InvalidArgument, "page token is invalid")
-		}
+		pageToken := pulid.PULID{}
+		if err := (&pageToken).Scan(bytes); err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "invalid argument: %s", err)
+		}
		listQuery = listQuery.
			Where(evcharger.IDLTE(pageToken))
	}

Alternatives

It would probably be cleaner to allow implementing UnmarshalProto/MarshalProto for custom types similar like we can implement a custom UnmarshalGQL/MarshalGQL. The reasoning is that the Valuer/Scanner interface used by the database driver is not necessarily what we want to represent in the protobuf types.

kdevo added 2 commits August 3, 2024 14:04
The current List service template generates faulty
code if a custom GoType field is used (currently only UUID and string
supported).

While not tested on a large set of inputs, using a call to the
`field_to_ent` template hopefully leads to a more sane behaviour,
as it is also used for Get() and Update() function calls.
@kdevo
Copy link
Contributor Author

kdevo commented Aug 3, 2024

Considering to change this to a draft. I just saw that this would introduce a breaking change as it results in normal UUIDs being interpreted as bytes directly instead of parsing them.

Any help/hint how to fix this with least impact or context why it is send as Base64-encoded string over the line is appreciated! I quickly went through #162 and I'm guessing it's because it is needed to accept any datatype as pageToken.

Edit: With the last commit, I parse the UUID, and then call field_to_ent with []byte. By doing so, we should preserve backward-compatibility. It seems hacky though (also due to byte-slicing twice in order to keep type compatibility), but I didn't want to touch other parts of the code yet to reduce the impact of the change. Is there any way to check if a GoType field option is used?

Probably the cleanest solution is the alternative mentioned in the opening comment but it's way more involved and it seems more reasonable to first fix the bug (even if not beautifully). Let me know what you think!

...when using a custom GoType.
This should keep the backward-compatibility by first parsing the UUID
represented as string (like before) and then calling `field_to_ent`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant